Reject proposals with duplicate actions in GovernorTimelockCompound#6457
Open
gabrielrondon wants to merge 3 commits intoOpenZeppelin:masterfrom
Open
Reject proposals with duplicate actions in GovernorTimelockCompound#6457gabrielrondon wants to merge 3 commits intoOpenZeppelin:masterfrom
gabrielrondon wants to merge 3 commits intoOpenZeppelin:masterfrom
Conversation
…imelockCompound Prevent proposals with duplicate actions (same target, value, calldata) from being submitted in GovernorTimelockCompound. The Compound timelock identifies queued transactions by hash, so duplicate actions within a proposal cause queueTransaction to fail. This override of _propose catches duplicates at proposal creation time with a clear GovernorDuplicateProposalAction error, rather than letting them fail later during queueing. Closes OpenZeppelin#6431
🦋 Changeset detectedLatest commit: 9966ae3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughThe changes introduce validation to prevent proposals with duplicate actions in the Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/governance/extensions/GovernorTimelockCompound.sol`:
- Around line 74-85: The nested duplicate-scan in
GovernorTimelockCompound._propose indexes values[i] and calldatas[i] assuming
all arrays match targets.length, which can cause out-of-bounds before the
canonical GovernorInvalidProposalLength check; ensure you validate that
values.length == targets.length and calldatas.length == targets.length (or call
the parent length-check) before running the duplicate loops so the existing
GovernorInvalidProposalLength revert fires first; update _propose to perform the
length validation up-front (or delegate to super._propose length checks) and
keep the duplicate detection afterwards, using the existing
GovernorDuplicateProposalAction and GovernorInvalidProposalLength symbols to
guide behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 35d79bf0-5f38-439b-a062-717f6950a019
📒 Files selected for processing (4)
contracts/governance/IGovernor.solcontracts/governance/extensions/GovernorTimelockCompound.solcontracts/mocks/governance/GovernorTimelockCompoundMock.soltest/governance/extensions/GovernorTimelockCompound.test.js
Comment on lines
+74
to
+85
| for (uint256 i = 1; i < targets.length; ++i) { | ||
| for (uint256 j = 0; j < i; ++j) { | ||
| if ( | ||
| targets[i] == targets[j] && | ||
| values[i] == values[j] && | ||
| keccak256(calldatas[i]) == keccak256(calldatas[j]) | ||
| ) { | ||
| revert GovernorDuplicateProposalAction(i); | ||
| } | ||
| } | ||
| } | ||
| return super._propose(targets, values, calldatas, description, proposer); |
Contributor
There was a problem hiding this comment.
Preserve canonical proposal-length validation before duplicate scan.
At Line 74, the nested loop indexes values[i]/calldatas[i] using targets.length. With malformed inputs, this can panic (out-of-bounds) before reaching Governor._propose’s GovernorInvalidProposalLength revert.
Proposed fix
function _propose(
address[] memory targets,
uint256[] memory values,
bytes[] memory calldatas,
string memory description,
address proposer
) internal virtual override returns (uint256) {
+ if (targets.length != values.length || targets.length != calldatas.length || targets.length == 0) {
+ revert GovernorInvalidProposalLength(targets.length, calldatas.length, values.length);
+ }
+
for (uint256 i = 1; i < targets.length; ++i) {
for (uint256 j = 0; j < i; ++j) {
if (
targets[i] == targets[j] &&
values[i] == values[j] &&
keccak256(calldatas[i]) == keccak256(calldatas[j])
) {
revert GovernorDuplicateProposalAction(i);
}
}
}
return super._propose(targets, values, calldatas, description, proposer);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/governance/extensions/GovernorTimelockCompound.sol` around lines 74
- 85, The nested duplicate-scan in GovernorTimelockCompound._propose indexes
values[i] and calldatas[i] assuming all arrays match targets.length, which can
cause out-of-bounds before the canonical GovernorInvalidProposalLength check;
ensure you validate that values.length == targets.length and calldatas.length ==
targets.length (or call the parent length-check) before running the duplicate
loops so the existing GovernorInvalidProposalLength revert fires first; update
_propose to perform the length validation up-front (or delegate to
super._propose length checks) and keep the duplicate detection afterwards, using
the existing GovernorDuplicateProposalAction and GovernorInvalidProposalLength
symbols to guide behavior.
Add array length validation at the start of _propose() in GovernorTimelockCompound, before the duplicate-scan loop. Without this, mismatched array lengths could cause an out-of-bounds access on values[] or calldatas[] before the canonical GovernorInvalidProposalLength check in super._propose() has a chance to fire.
…guards Add tests covering the early array-length validation in GovernorTimelockCompound._propose(): - empty proposal reverts with GovernorInvalidProposalLength - targets/values length mismatch reverts with GovernorInvalidProposalLength - targets/calldatas length mismatch reverts with GovernorInvalidProposalLength Also add a changeset entry for this PR. Signed-off-by: Gabriel Rondon <grondon@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #6431
Override
_proposeinGovernorTimelockCompoundto reject proposals containing duplicate actions at creation time. The Compound timelock identifies queued transactions by their hash, so duplicate actions within a single proposal cause the secondqueueTransactionto fail. This check catches the problem earlier with a clearer error message.Changes
IGovernor.sol: AddedGovernorDuplicateProposalAction(uint256 index)errorGovernorTimelockCompound.sol: Override_proposewith O(n²) pairwise comparison of(target, value, keccak256(calldata))tuplesGovernorTimelockCompoundMock.sol: Added required_proposeoverride to resolve diamond inheritanceGovernorTimelockCompound.test.js: Updated existing "duplicate calls" test to expect revert atpropose()instead ofqueue()Test plan
forge build— compiles cleannpx hardhat test test/governance/extensions/GovernorTimelockCompound.test.js— all 38 tests passingThis PR was developed with AI assistance (Claude).